This is what the perf tests report:

Baseline	This PR

In the past, I was told the PR's perf job was not reporting data for the own PR, so I am not sure I should trust these numbers? @felixarntz Should I still run my benchmark locally?

@oandregal the benchmarks reported in the screenshots that you've pasted are comparing WP 6.1.1 (the baseline) with your specific commit, so it's not measuring the impact of this specific change. This limitation is something we've got a ticket to address, but have not done so yet, so taking an individual benchmark if you can would be helpful. Thanks!

@felixarntz @joemcgill this is ready: created the trac ticket, provided testing instructions and a performance report.

Performance-wise, this has less impact than I originally thought. It is still valuable, though. Specially, because that the main reason for this PR is to introduce a public method to retrieve data from theme.json (consumers have to use private APIs so far).

Side-notes about performance report:

• It is still costly to prepare one. Would you think https://core.trac.wordpress.org/ticket/58358 could be prioritized? It's a bottleneck to analyze PRs manually. As a result, we do less pre-merge performance analysis. For example, in this PR, where it was not strictly necessary, I've spent a few hours.

• I'm surprised about the differences depending on the method. Not the numbers themselves, but the ratio for block vs classic themes. Codevitals reports server time as almost equal for classic and blocks while the other methods report classic themes being 2x faster than blocks (curl is more pessimistic and lies in 2.3x with benchmark-* tools being more optimistic around 1.9x).

@oandregal

I'm surprised about the differences depending on the method. Not the numbers themselves, but the ratio for block vs classic themes. Codevitals reports server time as almost equal for classic and blocks while the other methods report classic themes being 2x faster than blocks (curl is more pessimistic and lies in 2.3x with benchmark-* tools being more optimistic around 1.9x).

I have no idea, but that's definitely something we should research more. I wonder whether there's a notable difference in how the two environments set up the test sites? Otherwise, there's a good chance it is because the automated workflow used for Codevitals uses a headless browser, while benchmark-server-timing does not (similar to curl). I wonder if the headless browser setup adds some overhead that skews the data somehow - or vice versa?

🤔 I just realized the test data in use by CI is different from the “dummy” content that comes by default. I'd need to use that dataset to know whether it affected these tests.

I'm surprised about the differences depending on the method.

I just tried compare-wp-performance by @swissspidy and this is the result of running ./run.sh latest trunk.

wp-total (p50) for classic themes is at 20.88ms while it sits at 31.64ms for block themes, a 1.5x (unlike the ~1x of codevitals and ~2x of curl and benchmark-* tools). The little tools disagree with each other.

@felixarntz btw, all your feedback has been incorporated. Would you 🟢 this when you have the time, so I can continue follow-up work?

@oandregal Related to this change, @swissspidy shared some relevant feedback on how creating more and more individual wrapper functions for theme.json data is not exactly a sustainable approach, see Slack message. Wouldn't it be more reasonable to consider implementing caching directly into WP_Theme_JSON_Resolver, or implement a wrapper class that comes with caching?

@oandregal Related to this change, @swissspidy shared some relevant feedback on how creating more and more individual wrapper functions for theme.json data is not exactly a sustainable approach, see Slack message. Wouldn't it be more reasonable to consider implementing caching directly into WP_Theme_JSON_Resolver, or implement a wrapper class that comes with caching?

Happy to re-evaluate caching, though, we need the individual functions: they are the public API. Given the discussion around caching doesn't modify the API, I'm going to commit this to continue follow-up work. Please, let the conversation continue.

So, caching: the resolver already caches data. The issue is that it has to invalidate some of it once per request, resulting in some expensive processes having to run twice. I'm looking into changing how it works to make it run once. Take get_core_data as an example: the next image show all its callees to the right. They are called twice when they should have been called once:

By introducing the public APIs (and caching "at the edge" when it makes sense, such as for settings, styles, templateParts, etc.) I'm hoping to be able to decouple some of the internal processes that theme.json classes do, removing the unnecessary ones depending on the data being accessed, making sure caches are use effectively, etc.

Hope this helps?

A class can be a public API too, we don‘t have to add functions for everything. My concern was mostly that I‘m worried we‘ll end up creating X new functions each with its own caching wrapper around that class, leading to poor API design and lots of repetition
Hope that helps!

A class can be a public API too, we don‘t have to add functions for everything.

I know. Though, note that WP_Theme_JSON and WP_Theme_JSON_Resolver classes are indeed private, which means we cannot ask the consumers to use them for accessing data (normal use case).

My concern was mostly that I‘m worried we‘ll end up creating X new functions each with its own caching wrapper around that class, leading to poor API design and lots of repetition

I understand, those are valid concerns I also have. For reference, this is the plan. Note that the plan is not to have a micro-method for everything. The plan is to have five methods. One per top-level keys that can exists in a theme.json:

{
  "settings": {},
  "styles": {},
  "templateParts": {},
  "customTemplates": {},
  "patterns": {}
}

And the public API:

• wp_get_global_settings(). Shipped in 5.9.0.
• wp_get_global_styles(). Shipped in 5.9.0.
• wp_get_theme_directory_pattern_slugs. Shipped in 6.3.
• wp_get_theme_data_template_parts(). Added by this PR.
• wp_get_theme_data_custom_templates(). TBD.

The rationale for this is that each piece has its own requirements (translated or not translated, how sanitization works, etc.). By having these methods, we can improve how things work by refactoring the private classes/processes as needed without breaking backwards compatibility.

Does this overview alleviate some of your concerns?

Commited at https://core.trac.wordpress.org/changeset/56385